Skip to content

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Jan 3, 2022

fixes #8191

changelog: Fix needless_borrow causing mutable borrows to be moved
changelog: Rename ref_in_deref to needless_borrow
changelog: Suggest removing the borrow on method call receivers in needless_borrow

@rust-highfive
Copy link

r? @camsteffen

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 3, 2022
@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 2c9f1c3 to f48c0a8 Compare January 11, 2022 20:05
@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 11, 2022

This now includes all cases covered by ref_in_deref, which lints (&_).field. The description for both would lead one to believe they lint the same thing. Should I just rename it?

@camsteffen
Copy link
Contributor

I think we can remove/deprecate ref_in_deref then?

Tangentially, I noticed the other day that we don't lint (*x).y. I wonder where that could fit.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 11, 2022

I'm (very slowly) working on an explicit deref lint. It would fit in there whenever that's done.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 12, 2022

That successfully broke the changelog... Not really sure what to change there.

@camsteffen
Copy link
Contributor

You need to add declare_deprecated_lint! in clippy_lints/src/deprecated_lints.rs and also add a deprecation test in tests/ui/deprecated.rs. Then run cargo dev update_lints and it should be good.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 15, 2022

That would mean register_renamed can't be used, right?

@camsteffen
Copy link
Contributor

register_renamed would make it so any usage of ref_in_deref counts as needless_borrow. That may or may not be a good thing but I think the lints are different enough that we don't want that? In any case the error message should point to needless_borrow.

@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 15, 2022

The description for ref_inderef` is

Checks for references in expressions that use auto dereference.

The description for needless_borrow is

Checks for address of operations (&) that are going to be dereferenced immediately by the compiler.

Those describe exactly the same thing. o the point where anything covered by one and not he other would be a clear false negative. Given that renaming sound like the appropriate thing to do.

@camsteffen
Copy link
Contributor

camsteffen commented Jan 15, 2022

Okay, good point. So I think you can just add register_renamed by hand. To fix the error, change any ref_in_deref links in the changelog to not be links anymore.

@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 1ad870e to cb7fe3e Compare January 15, 2022 23:22
@Jarcho
Copy link
Contributor Author

Jarcho commented Jan 15, 2022

I was trying to avoid editing the changelog, but I don't see any other option.

@camsteffen
Copy link
Contributor

One idea for the lint message but otherwise looks good. Please update "changelog" to include the lint rename and added functionality.

@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 4cda9ab to e94a2e7 Compare January 21, 2022 14:57
@camsteffen
Copy link
Contributor

Squash some commits then will merge!

@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from e94a2e7 to ddc3caa Compare January 23, 2022 00:57
@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from ddc3caa to 5eae868 Compare January 23, 2022 01:35
@Jarcho Jarcho force-pushed the needless_borrow_8191 branch from 5eae868 to c615140 Compare January 23, 2022 02:22
@camsteffen
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2022

📌 Commit c615140 has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Jan 23, 2022

⌛ Testing commit c615140 with merge 788a8bc...

@bors
Copy link
Contributor

bors commented Jan 23, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 788a8bc to master...

@bors bors merged commit 788a8bc into rust-lang:master Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad suggestion for &mut *(&mut &mut T) from clippy::needless_borrow
4 participants